-
Notifications
You must be signed in to change notification settings - Fork 260
Shalapugin Dmitry #35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
krepysh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Обрати внимание на то как ты называешь переменные и функции. Имена должны быть понятными и объяснять что в переменной лежит или что функция делает.
По списком лучше итерироваться сразу по объектем, а не по индексу по длине.
Не используй i,j,k, etc как имена переменных в циклах, кроме как для индексов.
for_challenges.py
Outdated
| names = ['Оля', 'Петя', 'Вася', 'Маша'] | ||
| # ??? | ||
| for i in is_male: | ||
| if is_male[i] == True: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c True никогда не нужно сравнивать. просто if something:
for_challenges.py
Outdated
| } | ||
| names = ['Оля', 'Петя', 'Вася', 'Маша'] | ||
| # ??? | ||
| for i in is_male: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Позапускай код, он кажется не делает то что нужно. Я бы ожидал здесь увидеть другой for.
for_challenges.py
Outdated
| # ??? | ||
| a = 0 | ||
| print(f'Всего {len(groups)} группы.') | ||
| for i in groups: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for group in groups:
for_challenges.py
Outdated
| # ??? | ||
| a = 0 | ||
| print(f'Всего {len(groups)} группы.') | ||
| for i in groups: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| for i in groups: | |
| for group in groups: |
for_challenges.py
Outdated
| a = 0 | ||
| print(f'Всего {len(groups)} группы.') | ||
| for i in groups: | ||
| a += 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В питоне при итерировании списка принято использовать сразу элементы списка, а не индексы. Не используй индексы без острой необходимости.
| return messages | ||
|
|
||
|
|
||
| def zadacha(messages): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Разбей код на несколько максимально независимых функций, вынеси print наружу.
Назови переменные понятными именами: ab, ac, ac, i, j, k - плохие названия для переменных, потому что не показывают что в них лежит и как будут использоваться.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Код не разбит
string_challenges.py
Outdated
| # Вывести количество букв "а" в слове | ||
| word = 'Архангельск' | ||
| # ??? | ||
| print(len([j for j in ''.join(word) if j.lower() == 'а'])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
переусложнено - join тут вообще не нужен.
используй методы lower и count у str
string_challenges.py
Outdated
| # Вывести количество гласных букв в слове | ||
| word = 'Архангельск' | ||
| # ??? | ||
| print(len([j for j in ''.join(word) if j.lower() not in ['а', 'е', 'у', 'ы', 'о', 'э', 'я', 'и', 'ю']])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
вынеси список в перменную vowels
join не нужен
string_challenges.py
Outdated
| sentence = 'Мы приехали в гости' | ||
| # ??? | ||
| for i in ''.join(sentence).split(): | ||
| print(i[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
join не нужен
вместо i подошло бы имя переменной word
string_challenges.py
Outdated
| # Вывести усреднённую длину слова в предложении | ||
| sentence = 'Мы приехали в гости' | ||
| # ??? | ||
| q = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q?
krepysh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better!
Есть только замечание про поразбивать функции на функции попроще. Можешь потренироваться использовать в пайчарм рефакторинг:
Выдели курсором код который хочешь вынести в отдельную функцию, нажми правой кнопкой мыши - выбери Refactoring, используй Extract method функцию, Должна хорошо тут сработать.
for_challenges.py
Outdated
| names = ['Оля', 'Петя', 'Вася', 'Маша'] | ||
| # ??? | ||
| for name in names: | ||
| if name == 'Петя' or name == 'Вася': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
здесь надо использовать is_male.
А что если мы хотим больше имен добавить в программу. Должно быть одно место в программе ответственное за вычисление пола по имени.
for_challenges.py
Outdated
| # ??? | ||
| counter_1 = 0 | ||
| for group in groups: | ||
| counter_1 += 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Используй enumerate, вместо того что бы считать индекс руками
for_challenges.py
Outdated
| ] | ||
| # ??? | ||
| count_groups = 0 | ||
| for group in groups: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enumerate
for_dict_challenges.py
Outdated
|
|
||
| for klass in school: | ||
| print( | ||
| f"Класс {klass['class']}: мальчики {[is_male[men_person['first_name']] for men_person in klass['students']].count(True)} " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Вынеси функцию расчета числа мальчиков и девочек в отдельную функцию, так что бы можно было ее переиспользовать в следующих задачах.
| return messages | ||
|
|
||
|
|
||
| def zadacha(messages): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Код не разбит
string_challenges.py
Outdated
| # Вывести усреднённую длину слова в предложении | ||
| sentence = 'Мы приехали в гости' | ||
| # ??? | ||
| spisok = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| spisok = [] | |
| words = [] |
krepysh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Почти отлично.
Поправь эту штуку, что бы у тебя klass передавался аргументом явно. Очень плохая практика лазить неявно к чему-то вне твоей локальной области видимости.
| ] | ||
| # ??? | ||
|
|
||
| for group in enumerate(groups, start=1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
распакуй же сразу:
| for group in enumerate(groups, start=1): | |
| for group_number, group in enumerate(groups, start=1): |
И в остальных местах тоже. Эта техника называется list unpacking, кода ты паре переменных присваиваешь значение листа.
|
|
||
| count = 0 | ||
| for klass in school_students: | ||
| most_common_name = Counter(name['first_name'] for name in school_students[count]).most_common(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
school_students[count] это klass. Лучше использовать klass, и избавиться от count совсем.
| return [is_male[men_person['first_name']] for men_person in klass['students']].count(True) | ||
|
|
||
|
|
||
| def womens_count(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
klass должен передаваться явно как аргумент (можно еще и назвать его по другому, что бы точно не было сомнений.
No description provided.